Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable LIBOQS_DIR as cache entry and environment variable. #275

Closed
wants to merge 1 commit into from
Closed

Enable LIBOQS_DIR as cache entry and environment variable. #275

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 6, 2023 •
edited by ghost
Loading

Fix #273

liboqs_DIR is a CMake cache entry to hint CMake about the location of liboqs
CMake package.

However, being able to use LIBOQS_DIR (full capitalized) would be more
convenient.

This PR adds the ability to use either liboqs_DIR as a cache entry, liboqs_DIR
as an environment variable, LIBOQS_DIR as a cache entry and LIBOQS_DIR
as an env variable.

Tests were also added to make sure that these variables are well taken
by CMake during the configuration stage.

@ghost ghost force-pushed the liboqs_dir_env branch 4 times, most recently from 77aac2b to 7c39459 Compare October 6, 2023 10:26
@ghost ghost marked this pull request as ready for review October 6, 2023 10:30
@ghost ghost requested a review from baentsch as a code owner October 6, 2023 10:30
baentsch
baentsch reviewed Oct 6, 2023
View reviewed changes
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for all this work, particularly the new CI test for this "chose any capitalization" feature -- I'm just not sure why it's "green": When looking at it, it seems to have failed, e.g. open "Test oqsprovider" twisty at https://github.com/open-quantum-safe/oqs-provider/actions/runs/6430477110/job/17461513594 (?)

Enable LIBOQS_DIR as cache entry and environment variable.
2c56f3e 
Fix #273

`liboqs_DIR` is a CMake cache entry to hint CMake about the location of liboqs
CMake package.

However, being able to use `LIBOQS_DIR` (full capitalized) would be more
convenient.

This PR adds the ability to use either `liboqs_DIR` as a cache entry, `liboqs_DIR`
as an environment variable, `LIBOQS_DIR` as a cache entry and `LIBOQS_DIR`
as an env variable.

Tests were also added to make sure that these variables are well taken
by CMake during the configuration stage.
@ghost ghost force-pushed the liboqs_dir_env branch from 7c39459 to 2c56f3e Compare October 6, 2023 12:01
@ghost
Copy link
Author

ghost commented Oct 6, 2023

I'm testing cases where liboqs_DIR and LIBOQS_DIR mismatch (we don't want people to think that the bad value was taken because in fact they forgot to unsete LIBOQS_DIR for instance).
This is why we see some error strings here.

About

git config --global --add safe.directory /__w/oqs-provider/oqs-provider

I think it's due to the fact that I'm using REALPATH, which resolves symlinks.
I've switched to ABSOLUTE (which does not), let's see what happens!

@baentsch
Copy link
Member

baentsch commented Oct 6, 2023

let's see what happens!

Not the right thing :-/ Besides, why doesn't this get reported as a CI error? And: Would an actual build and test not be prudent?

@ajbozarth ajbozarth mentioned this pull request Oct 6, 2023
Closed
@ajbozarth
Copy link
Member

I've tried running this with multiple build commands and none of them work, even the one that did previously.

Usually I put the liboqs_DIR env var before the cmake command, but I also tried putting it inline as well (though this has never worked for me even on main).

Here's the four commands and tried and the error (red text) associated with each of them

  1. liboqs_DIR=$WORKSPACE/oqs cmake -DCMAKE_INSTALL_PREFIX=$WORKSPACE/oqs-provider -DOPENSSL_ROOT_DIR=$WORKSPACE -DCMAKE_BUILD_TYPE=Release -S . -B _build (this is the one that works on main)
CMake Error at CMakeLists.txt:101 (message):
  `libos_DIR` environment variable does not match the value previously set
  ('/Users/ajbozarth/workspace/quantum/openssl_build/oqs' !=
  '/Users/ajbozarth/workspace/quantum/oqs-provider').
  1. cmake -Dliboqs_DIR=$WORKSPACE/oqs -DCMAKE_INSTALL_PREFIX=$WORKSPACE/oqs-provider -DOPENSSL_ROOT_DIR=$WORKSPACE -DCMAKE_BUILD_TYPE=Release -S . -B _build
CMake Error at CMakeLists.txt:87 (message):
  `liboqs_DIR` cache entry and `LIBOQS_DIR` environment variable mismatch
  ('/Users/ajbozarth/workspace/quantum/openssl_build/oqs' !=
  '/Users/ajbozarth/workspace/quantum/oqs-provider').
  1. LIBOQS_DIR=$WORKSPACE/oqs cmake -DCMAKE_INSTALL_PREFIX=$WORKSPACE/oqs-provider -DOPENSSL_ROOT_DIR=$WORKSPACE -DCMAKE_BUILD_TYPE=Release -S . -B _build (note that this error is the one that I see when run options 2, 3 and 4 on main)
CMake Error at CMakeLists.txt:111 (find_package):
  By not providing "Findliboqs.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "liboqs", but
  CMake did not find one.

  Could not find a package configuration file provided by "liboqs" with any
  of the following names:

    liboqsConfig.cmake
    liboqs-config.cmake

  Add the installation prefix of "liboqs" to CMAKE_PREFIX_PATH or set
  "liboqs_DIR" to a directory containing one of the above files.  If "liboqs"
  provides a separate development package or SDK, be sure it has been
  installed.
  1. cmake -DLIBOQS_DIR=$WORKSPACE/oqs -DCMAKE_INSTALL_PREFIX=$WORKSPACE/oqs-provider -DOPENSSL_ROOT_DIR=$WORKSPACE -DCMAKE_BUILD_TYPE=Release -S . -B _build
CMake Error at CMakeLists.txt:90 (message):
  `LIBOQS_DIR` cache entry and `LIBOQS_DIR` environment variable mismatch
  ('/Users/ajbozarth/workspace/quantum/openssl_build/oqs' != '').

@baentsch
Copy link
Member

baentsch commented Oct 7, 2023

even the one that did previously.

Well, previously we were using cmake as intended. Now we try to do something going against its conventions. Apologies to @thb-sb to have suggested you "quickly" do this PR.

@ghost
Copy link
Author

ghost commented Oct 7, 2023

even the one that did previously.

Well, previously we were using cmake as intended. Now we try to do something going against its conventions. Apologies to @thb-sb to have suggested you "quickly" do this PR.

No problem.

Indeed we're trying to go against CMake conventions here. We may better keep the current variable as it follows CMake rules.

Another "solution": if LIBOQS_DIR is set, we could display a warning to redirect developers to liboqs_DIR.

Note that liboqs_DIR should be used as a CMake cache entry

@baentsch
Copy link
Member

baentsch commented Oct 7, 2023

We may better keep the current variable as it follows CMake rules.

Fully agree. Do you want to update this PR (maybe just add a documentation clarification instead of the new code) or just close it?

@ghost
Copy link
Author

ghost commented Oct 7, 2023

We may better keep the current variable as it follows CMake rules.

Fully agree. Do you want to update this PR (maybe just add a documentation clarification instead of the new code) or just close it?

I'll close this one since I've cross-referenced it with the issue.

Thank you!

@ghost ghost closed this Oct 7, 2023
@ghost ghost deleted the liboqs_dir_env branch October 7, 2023 09:38
@ajbozarth ajbozarth mentioned this pull request Oct 23, 2023
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The liboqs_DIR env var should be capitalized
2 participants
@baentsch @ajbozarth